Skip to content

Fix legacy CUDA TorchAO installs and automatic pin updates - #24728

Closed
zingo wants to merge 1 commit into
pytorch:mainfrom
zingo:Fix-legacy-CUDA-TorchAO-installs-and-automatic-pin-updates
Closed

zingo wants to merge 1 commit into
pytorch:mainfrom
zingo:Fix-legacy-CUDA-TorchAO-installs-and-automatic-pin-updates

Conversation

@zingo

@zingo zingo commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Use stable TorchAO for CUDA 12.6, whose nightly channel no longer receives builds. Exclude that channel from automatic nightly pin selection so it cannot hold back updates for CPU and CUDA 13.x.

Skip prebuilt TorchAO requirements when building from source. Allow source revision lookup for partially failed wheel workflows after required wheel availability has been checked.

Test plan

Add regression coverage and run the dependency tests in the existing lint workflow.

Authored with assistance from OpenAI Codex.

cc @Gasoonjia @digantdesai

Use stable TorchAO for CUDA 12.6, whose nightly channel no longer
receives builds. Exclude that channel from automatic nightly pin
selection so it cannot hold back updates for CPU and CUDA 13.x.

Skip prebuilt TorchAO requirements when building from source. Allow
source revision lookup for partially failed wheel workflows after
required wheel availability has been checked.

Add regression coverage and run the dependency tests in the existing
lint workflow.

Authored with assistance from OpenAI Codex.

Signed-off-by: Zingo Andersen <Zingo.Andersen@arm.com>
Change-Id: I428a7c788370a948038670c9f436e79c7204b70c
@pytorch-bot

pytorch-bot Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Pending, 3 Unrelated Failures, 1 Unclassified Failure

As of commit ccf1588 with merge base 12dac4a (image):

UNCLASSIFIED FAILURE - DrCI could not classify the following job because the workflow did not run on the merge base. The failure may be pre-existing on trunk or introduced by this PR:

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 6, 2026
@zingo zingo added the release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc. label Nov 6, 2026
@zingo
zingo requested a review from Gasoonjia November 6, 2026 12:11
@zingo zingo added ciflow/trunk help wanted Extra attention is needed labels Nov 6, 2026
@shoumikhin

Copy link
Copy Markdown
Contributor

@claude review effort=max

@claude

claude Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

Claude finished @shoumikhin's task in 27m 19s —— View job


PR Review: #24728

Request changes: the new lint step re-runs tests that pull.yml already runs and blocks lint output when they fail, the CUDA backend jobs don't run on this PR even though cu126 moves to TorchAO 0.18, and the cu126 rule is hardcoded in two modules while sharing the CPU-release TORCHAO_STABLE_VERSION pin.

Detailed review

Summary

This PR does four things:

  • CUDA 12.6 now installs the stable TorchAO release from whl/cu126.
  • The pin updater no longer includes cu126 when it picks the TorchAO nightly.
  • Source builds no longer pin a prebuilt TorchAO wheel.
  • The TorchAO commit lookup no longer requires status=success.

The problems: the new lint step can block lint output, the CUDA backend isn't tested against the new cu126 setup before merge, and the cu126 rule is duplicated and tied to the CPU-release pin. Smaller items follow. All findings come from reading the code; I couldn't run the PR's tests here.

Code Quality

  • The cu126 rule lives in two modules and shares the CPU-release pin.
    • install_requirements.py:48 (torch_url.endswith("/cu126")) and .github/scripts/update_pytorch_pin.py:127 ((major, minor) != (12, 6)) are separate copies of one rule. The comment at update_pytorch_pin.py:121 restates the other file's rule.
    • This will come up again: filter_cuda_matrix.py:62-64 says PyTorch keeps cu130 on nightlies only as a "temporary hold". If the next change misses the updater, the weekly bump is held back again, which is the bug this PR fixes. If it misses the installer, that train pins a nightly its channel doesn't have.
    • TORCHAO_STABLE_VERSION used to serve only the opt-in EXECUTORCH_USE_RELEASE_CPU_DEPS path, which an Ethos-U example uses. Now a +cu126 build of that version also has to exist on whl/cu126. The next TorchAO release probably won't have one, since cu126 nightlies stopped for the same reason. So bumping the constant for the CPU path would make cu126 x86 installs quietly fall back to the PyPI build.
    • Sharing the constant is also why get_torch_url (:56-58) now needs a second /cpu check.
    • Suggestion: list the stable-TorchAO trains once, next to SUPPORTED_CUDA_VERSIONS in install_utils.py. The updater already loads that file with runpy. Then give cu126 its own pin, like CU134_TORCHAO_NIGHTLY_VERSION and ROCM_TORCHAO_NIGHTLY_VERSION. Don't name it *TORCHAO_NIGHTLY_VERSION, or the updater's regex at update_pytorch_pin.py:197 will overwrite it. Fix this →
  • The torchao_url block repeats the aarch64 rule and still passes a dead index for source builds (install_requirements.py:110-125).
    • The stable branch repeats the aarch64→cpu check from the elif, and the comment at :117 now describes only one of the two. The three branches come down to a base (TORCHAO_RELEASE_URL_BASE or TORCHAO_NIGHTLY_URL_BASE) plus a channel (cpu on aarch64, otherwise the suffix of torch_url).
    • When torchao_from_source() is true, get_torchao_version returns the nightly version. cu126 x86 source builds therefore still get --extra-index-url .../whl/nightly/cu126, the channel this PR says is dead, even though :134 no longer asks for a TorchAO wheel. test_cu134_dependencies.py:197 asserts the same unused index for CPU source builds.
    • Fix: pass the TorchAO index only when a TorchAO wheel is requested. The cu134 local install at :207 still needs torchao_url. Fix this →
  • The commit lookup now accepts any scheduled run from that date, whatever its outcome (update_pytorch_pin.py:180-190).
    • get_latest_torchao_nightly proves only that wheels with that date stamp exist. It doesn't prove runs[0] built them.
    • A cancelled or startup_failure run is now accepted, where before the lookup raised. If that day's wheels came from a different run, third-party/ao gets pinned to the wrong SHA. I'm not confident this happens in practice, since there is usually one scheduled run per day.
    • Filtering on conclusion in ("success", "failure") in the script keeps the partial-failure fix and closes this gap.
    • The new test only checks that status=success is absent from the URL (test_update_pytorch_pin.py:56), so it passes either way. Fix this →

Infrastructure

  • The new lint step adds no coverage and blocks lint output when it fails (.github/workflows/lint.yml:113-116).
    • Both test files already run on every PR. pytest.ini:85 lists .ci/scripts/tests in testpaths, and the unittest job (pull.yml:894) has no if: and runs pytest -n auto from the repo root (unittest-linux-cmake.sh:35).
    • The step sits before "Run linters", which has no if: always(). Any failure in these tests skips lintrunner, so the PR gets no lint annotations and the lintrunner check fails for a reason that isn't lint.
    • Fix: drop the step, or move it to its own job if a fast signal is wanted. Fix this →

Testing

  • The CUDA backend jobs don't run on this PR.
    • cuda.yml's pull_request.paths (:26-50) don't include install_requirements.py, install_utils.py or setup.py, so test-executorch-cuda-build-12.6 doesn't run.
    • The only real cu126 install on this PR is the Vulkan LLM job test-model-vulkan-e2e, which runs because install_requirements.py changed (vulkan.yml:151,173). It covers the install and the LLM export quantization paths, but it doesn't build or test the CUDA backend.
    • After this PR, cu126 is the only default configuration on TorchAO 0.18 (setup.py:1139-1144 emits torchao>=0.18.0,<0.19 there). The other trains and third-party/ao stay on the 0.19 nightly. Code that needs a TorchAO API added in 0.19 then breaks only on cu126, and the CUDA backend jobs would first catch that after merge.
    • The Vulkan GPU jobs also run on CUDA 12.6 (vulkan.yml:82,173). After merge, their TorchAO tests (:135) run against 0.18 instead of the nightly.
    • Please add the ciflow/cuda label; the weekly bump already adds it (weekly-pytorch-pin-bump.yml:97).
  • Some unit tests are missing or fragile.
    • test_wheel_torchao_bound_matches_selected_train (test_cu134_dependencies.py:434) covers (13, 4), (13, 2) and None, but not the new cu126 bound.
    • test_update_pytorch_pin.py only passes when run from the repo root. get_supported_torchao_channels calls runpy.run_path("install_utils.py") relative to the current directory (update_pytorch_pin.py:120), while the sibling test file resolves paths from ROOT.
    • side_effect=versions.__getitem__ over a hardcoded dict (:28-34) raises KeyError as soon as a train is added to SUPPORTED_CUDA_VERSIONS. Fix this →

Specific Comments

  • .ci/scripts/tests/test_cu134_dependencies.py:186: the name test_cpu_source_build_keeps_nightly_torchao is stale; the test now asserts that no TorchAO wheel is pinned.
  • .github/scripts/update_pytorch_pin.py:384-385 and .github/workflows/weekly-pytorch-pin-bump.yml:85: "every supported CUDA channel" is no longer true now that cu126 is excluded.
  • install_requirements.py:19: TORCHAO_RELEASE_URL_BASE is also torch's release index (:59), so the TorchAO prefix is misleading.

Recommendation

Request Changes

Before merging:

  • Drop the lint step, or move it to its own job.
  • Run ciflow/cuda so the CUDA backend is tested against TorchAO 0.18 on cu126.
  • Define the cu126 rule and pin in one place, instead of in two modules plus the CPU-release constant.

The other items are not blockers.

Review process
  • Gathered context (PR diff vs merge base 12dac4a, CLAUDE.md, review checklist)
  • Ran three independent reviewers over all 5 changed files
  • Merged duplicate findings
  • Re-read the cited code for each finding; dropped the ones I couldn't confirm
  • Posted the review

I didn't run the tests: running commands locally wasn't permitted in this job. The CI status tool returned no runs for this PR.

Corrected after posting: the first version of this comment said no CUDA 12.6 install runs on this PR. One does: the Vulkan LLM job test-model-vulkan-e2e. The Testing finding now covers only the CUDA backend jobs, which really don't run.

@zingo zingo added module: cuda Issues related to the AOTI CUDA backend ciflow/cuda labels Nov 9, 2026
@pytorch-bot
pytorch-bot Bot deployed to upload-benchmark-results November 9, 2026 18:43 Active
@zingo

zingo commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Opps CUDA 12.6 support was removed now #24740 Ill see if there is anything we still want to keep and rework the PR , or just close it. Lets see :) .

@zingo

zingo commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator Author

Lets close and Ill create a new PR with the leftovers.

@zingo zingo closed this Nov 10, 2026

This branch was successfully deployed

1 active deployment
upload-benchmark-results — ccf1588d Deployed Nov 9, 2026 by pytorch-bot[bot] via upload-benchmark-results #22178
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/cuda ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. help wanted Extra attention is needed module: cuda Issues related to the AOTI CUDA backend release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants